-
Notifications
You must be signed in to change notification settings - Fork 72
fix: Ensure request.url is with correct protocol in route handlers #865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 148d057 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @sommeeeer for fixing this long standing issue.
I have added a couple of minor comments inline. The PR is good to go when those are addressed.
Edit: and kudos for the PR description too. Adding context is super helpful to ease the review 🙏
Co-authored-by: Victor Berchet <[email protected]>
Thanks for the review Victor! |
This is for opennextjs/opennextjs-aws#969
We patch NextServer's
attachRequestMeta
as it will be called inhandleRequestImpl
.In theory we can also just comment out that line as we pass ininitURL
tonextServer.getRequestHandlerWithMetadata
arguments already here when we process the request through NextServer.Has been tested on Next 14 aswell.
I did update the tests that relied on this aswell.
Update: Turns out in pages router it relies on
initURL
to be this template string value. Reason is that it wont have the_next/data
part in theinitURL
we passed in to the request handlers metadata, however it will be inreq.url
. Because of that I just updated the fix to only replace the protocol instead.